Triggered Lightning: Implement Layer Mean Temperature Plugins#2321
Conversation
…-lightning-layer-mean-temperature-plugin
robertplatt-mo
left a comment
There was a problem hiding this comment.
@Anzerkhan27 I've requested a few changes the main comments being in layer_mean_temperature and probably the most time consuming being type hints which I understand improver style guide expects.
Generally it all looks pretty sensible though.
| (base, interior, and top). | ||
| """ | ||
| if bottom >= top: | ||
| raise ValueError(f"bottom ({bottom} ft) must be less than top ({top} ft).") |
There was a problem hiding this comment.
| raise ValueError(f"bottom ({bottom} ft) must be less than top ({top} ft).") | |
| raise ValueError(f"Bottom ({bottom} ft) must be less than top ({top} ft).") |
| from improver import BasePlugin | ||
|
|
||
|
|
||
| class LayerExtractionAndInterpolation(BasePlugin): |
There was a problem hiding this comment.
I'm not keen on the name of this plugin. Firstly extract and interpolate feels like it does two things. Generally it's good if functionality can be explained as doing one thing. Extraction seems to me to be something we do in service of the more important interpolation. The name also doesn't communicate that the method is primarily a tool for temperature interpolation at layer boundaries. This makes the tool seem like it's generic when it surely is not.
How about BoundaryTemperatureInterpolation?
There was a problem hiding this comment.
Or LayerTemperatureInterpolation or TemperatureInterpolationAtLayers?
There was a problem hiding this comment.
I think you are correct, the main overarching thing the function does here is Interpolate temperature at given heights. The extraction depends upon the boundary arguments that we provide. So it make sense to rename the plugin to LayerTemperatureInterpolation
| Extract and interpolate temperature values at layer boundaries. | ||
|
|
||
| Args: | ||
| temp_cube (iris.cube.Cube): Input temperature cube with a height coordinate. |
There was a problem hiding this comment.
Ryan has previously informed me that the way improver docs are generated we don't need type hints in the args as these can be dynamically generated for the types (for more info on this though probably best to ask him).
I noticed this and then subsequently noticed we haven't got any type hints. My understanding is the improver style guide has these as a requirement. Please consider this comment to apply to all docstrings and all methods (sorry I know it's a pain).
There was a problem hiding this comment.
Type hints added to method signature
| iris.Constraint( | ||
| height=lambda point: bottom / self.metres_to_ft | ||
| < point | ||
| < top / self.metres_to_ft |
There was a problem hiding this comment.
As noted above I don't think we should be passing this in (but please correct me if I'm mistaken and there is a good reason I am not aware of). I'd set it at the top of the file as a constant.
There was a problem hiding this comment.
I have change it to a constant added at the top of file
| if verbosity: | ||
| print("Layer mean temperature array:", lmt_array) | ||
|
|
||
| # Wrap result in a cube (add metadata as needed) |
There was a problem hiding this comment.
| # Wrap result in a cube (add metadata as needed) | |
| # Wrap result in a cube and add required metadata |
As needed rather implies the user will choose what metadata is added imv
There was a problem hiding this comment.
Thank you pointing this out. I have made the change
Thank you @robertplatt-mo . I have added the |
| """ | ||
| height_points = np.array([100, 200, 300], dtype=np.float32) | ||
| y_points = np.array([0, 1], dtype=np.float32) | ||
| x_points = np.array([0, 1], dtype=np.float32) |
There was a problem hiding this comment.
If lines 22-24 were customised to the shape of the input data, then
make_layer_cube would be more general. I think make_layer_cube
could be used in more places in the unit tests.
improver_tests/temperature/layer_mean_temperature/test_layer_temperature_interpolation.py
Show resolved
Hide resolved
improver_tests/temperature/layer_mean_temperature/test_layer_temperature_interpolation.py
Show resolved
Hide resolved
correct a docstring comment
mo-DavidJohnJohnston
left a comment
There was a problem hiding this comment.
Added an extra test after discussions with Anzer to check the mean layer temperature
when there is a varying temperature field. The existing check of the output value is for a
constant value fields.
to combat different versions of ruff
…yer-mean-temperature-plugin' into EPPT-2747-triggered-lightning-layer-mean-temperature-plugin
mo-DavidJohnJohnston
left a comment
There was a problem hiding this comment.
Changes discussed with Anzer have been made.
One extra unit test was needed to cover the case
when the temperature field is not constant.
I approve the PR.
|
|
||
| from improver import BasePlugin | ||
|
|
||
| METRES_TO_FT = 3.28084 |
There was a problem hiding this comment.
Just a comment, to avoid this hardcoded conversion factor, you could use the cf_units package i.e. https://cf-units.readthedocs.io/en/latest/unit.html#cf_units.Unit.convert to handle the unit conversion, which we use elsewhere in IMPROVER.
There was a problem hiding this comment.
Thank you for the comment @gavinevans. I added cf_units to handle unit conversion, it looks cleaner.
Thank your adding in the missing test and fixing docstrings |
Resolved the requested changes
mo-DavidJohnJohnston
left a comment
There was a problem hiding this comment.
The latest change to use cf_units for conversion is an improvement.
I have not checked the unit tests this time, but I am assuming this is a
formality, and I approve the PR.
…emperature-plugin
EPPT-3150
This PR implements the mean layer temperature calculation step as part of the ongoing migration of the Helicopter-Triggered Lightning (GPP → EPP) workflow (
eppuk_triggered_lightning).The GPP implementation contained a single monolithic function
calculate_layer_mean_temperature()which handled both the vertical layer extraction and the weighted mean calculation in one function. This PR introduces two IMPROVER-style plugins to replace this, following EPP conventions.Two new plugins have been implemented:
1.
LayerExtractionAndInterpolationiris.analysis.Linear().2.
CalculateLayerMeanTemperatureLayerExtractionAndInterpolationas input.projection_y_coordinate,projection_x_coordinate) with scalar coordinatesforecast_period,forecast_reference_time, andtimepreserved from the input.standard_name="air_temperature"(CF convention) withunits="K".